Remove usage of Project during task execution #455

liutikas · 2023-03-01T20:44:07Z

Replace project with injected FileOperations object
Replace project with logger as that's the only thing used

Fixes part of GoogleCloudPlatform/appengine-plugins#997 issue

Test: existing tests pass

- Replace project with injected FileOperations object - Replace project with logger as that's the only thing used Fixes part of #454 issue Test: existing tests pass

vlsi

Please do not use .gradle..internal.. classes
FileSystemOperations requires Gradle 6.0, so either the implementation should be adjusted to support Gradle 5, or someone should decide if bumping the minimal version to 6.0 was acceptable. If the minimal Gradle version is increased, then README should be updated as well to reflect the change.

vlsi · 2023-03-27T06:14:16Z

src/main/java/com/google/cloud/tools/gradle/appengine/appyaml/StageAppYamlTask.java

+public abstract class StageAppYamlTask extends DefaultTask {
+
+  @Inject
+  abstract public FileOperations getFileOperations();


FileSystemOperations requires Gradle 6.0+
Recently there was a commit comparing the version with 5.5 (see #451 ), so I'm not sure if bumping the minimal version to Gradle 6.0 is generally expected from an innocent change like this

Suggested change

abstract public FileOperations getFileOperations();

abstract public FileSystemOperations getFileSystemOperations();

Don't take my usage of 5.1 as a guide :) I used the lowest possible point where the new API is available. During maintenance this makes the condition disappear faster than if it's at the point of deprecation.

A more relevant discussion might be: #450

So the minimum supported version is Gradle 4.0, which means this suggestion can't work.

I think the getFileOperations() needs a

// TODO use public org.gradle.api.file.FileSystemOperations once the minimum version is greater than Gradle 6.0.

but until then it's safe to use as the FileOperations API, however internal, was added in Gradle 0.9 and hasn't changed since then (for this use case).

Alternatively we could use some other library to do the same. e.g.

org.gradle.util.GFileUtils.deleteDirectory(appYamlExtension.getStagingDirectory());

or

org.apache.tools.ant.util.FileUtils.delete(appYamlExtension.getStagingDirectory());

or plain old Java 8 (assuming minimum supported version allows, what is the minimum Java for this plugin? Gradle 4.0 supports Java 7 minimum):

Files.walk(pathToBeDeleted) .sorted(Comparator.reverseOrder()) .map(Path::toFile) .forEach(File::delete);

and since the .delete() just deleted the folder it's possible to just use plain appYamlExtension.getStagingDirectory().mkdir().

vlsi · 2023-03-27T06:15:15Z

src/main/java/com/google/cloud/tools/gradle/appengine/appyaml/StageAppYamlTask.java

 import org.gradle.api.DefaultTask;
+import org.gradle.api.internal.file.FileOperations;


Please do not import org.gradle.api.internal packages. Those are not meant for general use in plugin code

Suggested change

import org.gradle.api.internal.file.FileOperations;

import org.gradle.api.file.FileSystemOperations;

It's not possible to use the public API here, because it was added in Gradle 6.0 as you said.

TWiStErRob

This PR is almost ready. I reviewed again deeper and it looks good, googleJavaFormat is failing the build with trivial problems. I commented what needs changing/deciding to get this merged.

TWiStErRob · 2023-08-04T01:59:33Z

src/main/java/com/google/cloud/tools/gradle/appengine/appyaml/StageAppYamlTask.java

 import org.gradle.api.DefaultTask;
+import org.gradle.api.internal.file.FileOperations;


It's not possible to use the public API here, because it was added in Gradle 6.0 as you said.

TWiStErRob · 2023-08-04T02:01:33Z

src/main/java/com/google/cloud/tools/gradle/appengine/appyaml/StageAppYamlTask.java

+public abstract class StageAppYamlTask extends DefaultTask {
+
+  @Inject
+  abstract public FileOperations getFileOperations();


So the minimum supported version is Gradle 4.0, which means this suggestion can't work.

I think the getFileOperations() needs a

// TODO use public org.gradle.api.file.FileSystemOperations once the minimum version is greater than Gradle 6.0.

but until then it's safe to use as the FileOperations API, however internal, was added in Gradle 0.9 and hasn't changed since then (for this use case).

Alternatively we could use some other library to do the same. e.g.

org.gradle.util.GFileUtils.deleteDirectory(appYamlExtension.getStagingDirectory());

or

org.apache.tools.ant.util.FileUtils.delete(appYamlExtension.getStagingDirectory());

or plain old Java 8 (assuming minimum supported version allows, what is the minimum Java for this plugin? Gradle 4.0 supports Java 7 minimum):

Files.walk(pathToBeDeleted) .sorted(Comparator.reverseOrder()) .map(Path::toFile) .forEach(File::delete);

and since the .delete() just deleted the folder it's possible to just use plain appYamlExtension.getStagingDirectory().mkdir().

TWiStErRob · 2023-08-04T02:03:54Z

src/main/java/com/google/cloud/tools/gradle/appengine/appyaml/StageAppYamlTask.java

+public abstract class StageAppYamlTask extends DefaultTask {
+
+  @Inject
+  abstract public FileOperations getFileOperations();


gradlew googleJavaFormat wants this modifier order:

Suggested change

abstract public FileOperations getFileOperations();

public abstract FileOperations getFileOperations();

TWiStErRob · 2023-08-04T02:04:25Z

...n/java/com/google/cloud/tools/gradle/appengine/core/DownloadCloudSdkTaskConsoleListener.java

@@ -20,12 +20,13 @@
 import com.google.cloud.tools.managedcloudsdk.ConsoleListener;
 import org.gradle.api.Project;


gradlew googleJavaFormat found an unused import here, which makes sense since you removed the field.

Remove usage of Project during task execution

3e26e26

- Replace project with injected FileOperations object - Replace project with logger as that's the only thing used Fixes part of #454 issue Test: existing tests pass

liutikas mentioned this pull request Mar 1, 2023

Make app-gradle-plugin configuration cache compatible GoogleCloudPlatform/appengine-plugins#997

Open

loosebazooka approved these changes Mar 6, 2023

View reviewed changes

vlsi suggested changes Mar 27, 2023

View reviewed changes

Merge branch 'master' into projectduringexec

198c45e

TWiStErRob approved these changes Aug 4, 2023

View reviewed changes

JoeWang1127 closed this Jan 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove usage of Project during task execution #455

Remove usage of Project during task execution #455

liutikas commented Mar 1, 2023

vlsi left a comment

vlsi Mar 27, 2023

TWiStErRob Mar 27, 2023 •

edited

Loading

TWiStErRob Aug 4, 2023 •

edited

Loading

vlsi Mar 27, 2023

TWiStErRob Aug 4, 2023

TWiStErRob left a comment •

edited

Loading

TWiStErRob Aug 4, 2023

TWiStErRob Aug 4, 2023 •

edited

Loading

TWiStErRob Aug 4, 2023

TWiStErRob Aug 4, 2023

	abstract public FileOperations getFileOperations();
	abstract public FileSystemOperations getFileSystemOperations();

		import org.gradle.api.DefaultTask;
		import org.gradle.api.internal.file.FileOperations;

	import org.gradle.api.internal.file.FileOperations;
	import org.gradle.api.file.FileSystemOperations;

	abstract public FileOperations getFileOperations();
	public abstract FileOperations getFileOperations();

		@@ -20,12 +20,13 @@
		import com.google.cloud.tools.managedcloudsdk.ConsoleListener;
		import org.gradle.api.Project;

Remove usage of Project during task execution #455

Remove usage of Project during task execution #455

Conversation

liutikas commented Mar 1, 2023

vlsi left a comment

Choose a reason for hiding this comment

vlsi Mar 27, 2023

Choose a reason for hiding this comment

TWiStErRob Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

TWiStErRob Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

vlsi Mar 27, 2023

Choose a reason for hiding this comment

TWiStErRob Aug 4, 2023

Choose a reason for hiding this comment

TWiStErRob left a comment • edited Loading

Choose a reason for hiding this comment

TWiStErRob Aug 4, 2023

Choose a reason for hiding this comment

TWiStErRob Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

TWiStErRob Aug 4, 2023

Choose a reason for hiding this comment

TWiStErRob Aug 4, 2023

Choose a reason for hiding this comment

TWiStErRob Mar 27, 2023 •

edited

Loading

TWiStErRob Aug 4, 2023 •

edited

Loading

TWiStErRob left a comment •

edited

Loading

TWiStErRob Aug 4, 2023 •

edited

Loading